Skip to content

Tweak colors#5063

Merged
1ucian0 merged 28 commits intoQiskit:masterfrom
galeinston:Tweak_Colors_IQX
Sep 28, 2020
Merged

Tweak colors#5063
1ucian0 merged 28 commits intoQiskit:masterfrom
galeinston:Tweak_Colors_IQX

Conversation

@galeinston
Copy link
Contributor

Summary

Fixes #5032

Details and comments

Modified background colors of operational gates and their font text colors to match with IQX colors as suggested by @1ucian0 in the original issue.

Copy link
Contributor

@enavarro51 enavarro51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1ucian0 Need clarification on some colors.
Gate Previous New
cx Blue DarkBlue
ccx Purple DarkBlue
mcx Purple ?
swap Blue DarkBlue
cswap Purple DarkBlue
ccswap Purple ?
dcx Blue ?
iswap Blue ?
What about u and p?
I assume all controlled versions of t, s, sdg, and tdg will be DarkRed?
I also assume all other gates - custom to_gates, isometry, diagonal, gms, etc., etc. will be DarkRed?
Thanks.

Comment on lines 928 to 936
# ccx gates
elif isinstance(op.op, ControlledGate) and op.name == 'ccx':
num_ctrl_qubits = op.op.num_ctrl_qubits
self._set_ctrl_bits(op.op.ctrl_state, num_ctrl_qubits,
q_xy, ec=ec, tc=tc, text=ctrl_text, qargs=op.qargs)
self._x_tgt_qubit(q_xy[num_ctrl_qubits+1], ec=ec,
ac=self._style.dispcol['target'])
self._line(qreg_b, qreg_t, lc=lc)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this elif is necessary. The ccx is covered by the base_name == 'x' elif above it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I don't specifically add the elif here, the color would be in dark red instead of dark blue (please check the reference in #5032). The present version doesn't handle this scenario either, i.e. >=2 controlled qubit, the color is different from colors defined for swap and cx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you added ccx to dispcol in qcstyle. This should take care of the color. I haven't run your code yet, but when I add it in to qcstyle with a different color using the master version, the color changes in the display output.

Comment on lines 950 to 966
elif op.name != 'swap' and base_name == 'swap':
elif op.name == 'cswap':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not cover swap gates with more than one control, like ccswap, so should go back to original.

Copy link
Contributor Author

@galeinston galeinston Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I also figured this problem. The current version also doesn't cover for cases with more than 1 controlling qubit. Since the original problem asked for colors change only, I modified the code to match it up with IQX reference. Should we create a different issue for it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you did it, but we have to be careful we don't lose functionality. Adding a separate elif would be better. I left a comment on the original issue. I feel like we're working with an incomplete spec.

Copy link
Collaborator

@Cryoris Cryoris Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specifications are complete, see the comment below: #5063 (comment)

@1ucian0
Copy link
Member

1ucian0 commented Sep 15, 2020

@1ucian0 Need clarification on some colors.

I have no opinion here. @nonhermitian ?

@galeinston
Copy link
Contributor Author

galeinston commented Sep 15, 2020

@1ucian0 Since the color outline is still under discussion. Should I run the binder as in #4544 or put it on hold?

Another question: there are 2 active PRs right now to solve #5032. Should we keep updating our code simultaneously like this?

@Cryoris
Copy link
Collaborator

Cryoris commented Sep 16, 2020

@enavarro51 @1ucian0 here's an overview of the colors: https://quantum-computing.ibm.com/docs/iqx/operations-glossary.

Classical gates are dark blue, this includes also MCX and DCX, since they are also a classical operation.
Phase gates (this means diagonal matrices) are light blue, this includes also P, T and S as well as their inverses. Non-unitary operations, such as c_if, barrier, measure and reset are grey. All other quantum gates are dark red, i.e. custom to_gate etc.

So here's the overview again, including the gates @enavarro51 mentioned above:

Classical: Dark blue
I, X, CX, CCX, C3X, C4X, any MCX, DCX, Swap, CSwap, and controlled versions of these.

Phase gates: Cyan
Z, S, Sdg, T, Tdg, P, U1

Hadamard: Red
H

Non-unitary: Grey
Reset, c_if, Barrier, Measure

Other: Dark red
Every gate not mentioned above, also arbitrary custom gates, U, U3, GMS, SX, etc.

In principle, controlled versions of the phase gates should also be cyan as controlling a diagonal gates is still diagonal. But this might be a little more complicated to do and we can stick to just making the single qubit ones cyan for now and the controlled versions have the default red.

@Cryoris
Copy link
Collaborator

Cryoris commented Sep 16, 2020

Another question: there are 2 active PRs right now to solve #5032. Should we keep updating our code simultaneously like this?

I don't think this makes a lot of sense, it'll just duplicate the work and we'll be only able to merge one 🤔

Comment on lines 25 to 33
basis_color = '#FA74A6' # Red
clifford_color = '#6FA4FF' # Blue
iden_color = '#05BAB6' # Green

hadamard_color = '#DC143C' # Red
classical_gate_color = '#000080' # Dark Blue
phase_gate_color = '#1E90FF' # Light Blue
non_unitary_gate_color = '#808080' # Grey
other_color = '#99004C' # Dark Red
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the official color codes, could you update these?

classical: #002D9C
phase: #33B1FF
hadamard: #FA4D56
quantum: #9F1853
non-unitary: #A8A8A8

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cryoris absolutely

Comment on lines 345 to 353
gt = self._style.lc
else:
gt = self._style.gt
if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'reset', 'meas', 'measure'}:
gt = self._style.not_gate_lc
self._style.sc = self._style.not_gate_lc
else:
gt = self._style.gt
self._style.sc = self._style.lc
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text colors should not be hardcoded in matplotlib.py but should be a dictionary in the Style class like the gate color. Otherwise we won't be able to use different style sheets that use different combinations of gate and text color.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I'll fix it

@enavarro51
Copy link
Contributor

@Cryoris Thanks for the great info. This clarifies a lot.

@enavarro51
Copy link
Contributor

I have a proposal that will get us well on the way to a true user-defined colormap/style sheet and can be done as an extension of this PR.

There already exists a rudimentary style sheet idea in the mpl drawer, but it is incomplete and buggy. In fact, I'm relatively sure there is nobody using the style capability for 'displaycolor' since it would only work if they knew the source and knew one specific way to set it up. It would be fairly easy to make the 'displaycolor' option available to users.

In addition there is a user-configurable selection in the settings.conf file for 'circuit_mpl_style', but the only option that works in the mpl code is 'default', which is what you would get anyway. So I think we should add 'original' - for current colors, 'iqx' - for the new iqx color map, and 'bw' for black & white. Then we just need 3 style classes in qcstyle to do it all.

Also there is an issue with font color, since there are a lot of border cases where black or white could both work, so I'd like to make the dispcol value a (color, font_color) tuple which would make the qcstyle code much easier to maintain. Unlike the old version of mpl, which had 35 references to dispcol, the new version has 3 (pat myself on the back :)), so updating this in mpl would be easy as well, and can be made backward compatible, in case anyone was using the 'displaycolor' option.

This would be phase 1 on the path to full user colormap definition. It would,

  • Provide 3 color styles, original, iqx, and bw, selectable by the user
  • Create a structure for future imports of user-styles
  • Allow the user to configure specific gate colors through the style 'displaycolor' param to draw

Phase 2 would create a location for colormaps, maybe .qiskit, and let users build whatever they wanted and import somehow into mpl, either as an import or a file read.

I'm happy to work with @galeinston on this. It really should be fairly straightforward.

@galeinston
Copy link
Contributor Author

I like the idea of 3 color styles and how @enavarro51 planned out to make it work. It's be my pleasure to work on the proposed extension.

@mtreinish
Copy link
Member

I have a proposal that will get us well on the way to a true user-defined colormap/style sheet and can be done as an extension of this PR.

There already exists a rudimentary style sheet idea in the mpl drawer, but it is incomplete and buggy. In fact, I'm relatively sure there is nobody using the style capability for 'displaycolor' since it would only work if they knew the source and knew one specific way to set it up. It would be fairly easy to make the 'displaycolor' option available to users.

FWIW, we do try to document the style sheet style parameter which does let users define a full style dict/ json for the drawer to use: https://qiskit.org/documentation/stubs/qiskit.visualization.circuit_drawer.html#qiskit.visualization.circuit_drawer But I agree it could definitely use some love.

In addition there is a user-configurable selection in the settings.conf file for 'circuit_mpl_style', but the only option that works in the mpl code is 'default', which is what you would get anyway. So I think we should add 'original' - for current colors, 'iqx' - for the new iqx color map, and 'bw' for black & white. Then we just need 3 style classes in qcstyle to do it all.

I agree with this, instead of changing the default this PR should be adding a 3rd iqx class to qcstyle.py. For the settings.conf we should just iqx to list of valid default styles in qiskit/user_config.py I think what you're calling original here should probably stay as default (just for backwards compat reasons. Besides the settings.conf file, default is the kwarg default value for the draw functions. If we do rename default to original we'll still have to correctly handle when default is set as the value in the settings.conf and kwarg.

Also there is an issue with font color, since there are a lot of border cases where black or white could both work, so I'd like to make the dispcol value a (color, font_color) tuple which would make the qcstyle code much easier to maintain. Unlike the old version of mpl, which had 35 references to dispcol, the new version has 3 (pat myself on the back :)), so updating this in mpl would be easy as well, and can be made backward compatible, in case anyone was using the 'displaycolor' option.

This would be phase 1 on the path to full user colormap definition. It would,

* Provide 3 color styles, original, iqx, and bw, selectable by the user

* Create a structure for future imports of user-styles

* Allow the user to configure specific gate colors through the style 'displaycolor' param to draw

I think this last point might be out of scope for this PR. I'm assuming by this you're talking about doing something like:

circuit_drawer(qc, output='mpl', style={'displaycolor': {'h': '#ff0080'}})

and instead of the current behavior where only hadamard gates would have a color set, it changes the default color in the configured style sheet to use the fuschia for hadmard and everything remains the same. We should do this piece separately because there are backwards compat implications for changing this behavior. We can quickly add a new stylesheet to the drawer for the iqx theme (and fix the font color thing as you described) without this piece.

Phase 2 would create a location for colormaps, maybe .qiskit, and let users build whatever they wanted and import somehow into mpl, either as an import or a file read.

My thinking on this was to either do it one of 2 ways:

  1. We add a mpl_stylesheet dir to ~/.qiskit (and allow it to be configured via an env var and entry in the settings.conf). Then we just locate all the .json files in that dir and use the filename as the style name.
  2. We add a config section to the settings.conf for mpl style sheetsheet where we add entries for style name and path to json. For example:
[mpl_stylesheets]
custom1 = ~/custom_mpl_style.json
custom2 = ~/special_mpl_style.json

I'm leaning more towards the later, because it also gives us this flexibility for more easily distinguishing style sheets between the mpl circuit drawer, the pulse drawer (and possibly the mpl state visualizations too). The pulse visualizer has the same kind of style sheets as the circuit drawer and whatever we expose for the circuit drawer we're also going to want to work for the pulse visualizer too.

@enavarro51
Copy link
Contributor

I agree with this, instead of changing the default this PR should be adding a 3rd iqx class to qcstyle.py. For the settings.conf we should just iqx to list of valid default styles in qiskit/user_config.py I think what you're calling original here should probably stay as default (just for backwards compat reasons. Besides the settings.conf file, default is the kwarg default value for the draw functions. If we do rename default to original we'll still have to correctly handle when default is set as the value in the settings.conf and kwarg.

Using 'default' is fine.

I think this last point might be out of scope for this PR. I'm assuming by this you're talking about doing something like:
circuit_drawer(qc, output='mpl', style={'displaycolor': {'h': '#ff0080'}})
and instead of the current behavior where only hadamard gates would have a color set, it changes the default color in the configured style sheet to use the fuschia for hadmard and everything remains the same. We should do this piece separately because there are backwards compat implications for changing this behavior. We can quickly add a new stylesheet to the drawer for the iqx theme (and fix the font color thing as you described) without this piece.

There actually is not a backwards compat problem here. Someone can currently do what you show above, but if they put an 'x' gate in the circuit, the drawer would crash. The 'displaycolor' dict currently has to have an entry for every gate that's used. Not sure anybody really knows that.

What I'm proposing is doing the same thing, but not crashing. As you say, this would then change the 'h' color and leave all the rest the same. Don't think we need to worry about backwards compat if the previous behavior was a crash.

And making the (color, font_color) tuple can be backwards compat as well by checking if the value for 'h' is a string or tuple in the code. If a string, use the color and make the font_color either the default or what the user has defined in the style dict.
So the new way would now look like,
circuit_drawer(qc, output='mpl', style={'displaycolor': {'h': ('#ff0080', '#000000')}})
It's not real elegant for the user, but the capability is there and anything they were previously doing would still work.

All of this will require some doc changes, of course.

As to the phase 2, what you suggest looks fine and making it compatible with the pulse drawer would be great. Once this PR is done, we can create a new issue for phase 2.

@enavarro51
Copy link
Contributor

@galeinston Are you on qiskit Slack?

@galeinston
Copy link
Contributor Author

Yup. I'm on Slack

@galeinston
Copy link
Contributor Author

galeinston commented Sep 17, 2020

Following @1ucian0 suggestion in #4544,I made this binder to test the MPL. I believe this link is still valid even after @enavarro51 and I update the code based on #5063 (comment) and #5063 (comment)
👇
Binder

@enavarro51
Copy link
Contributor

Phase gates: Cyan
Z, S, Sdg, T, Tdg, P, U1

@Cryoris Just one more check. What about sx and sxdg. Are these phase gates? Thanks.

@abhobe
Copy link
Contributor

abhobe commented Sep 18, 2020

@galeinston Make sure to update the refs in /test/ipynb/mpl/references to match your gate colors. Or else you will have failing binder tests.

Copy link
Contributor

@abhobe abhobe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some small changes.

gt = self._style.lc
else:
gt = self._style.gt
if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'r', 'reset', 'meas', 'measure'}:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'r', 'reset', 'meas', 'measure'}:
if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'reset', 'meas', 'measure'}:

I think r should be removed because it seems like its a quantum gate which needs a white font.

Binder Tests
image

@Cryoris
Copy link
Collaborator

Cryoris commented Sep 18, 2020

@Cryoris Just one more check. What about sx and sxdg. Are these phase gates? Thanks.

@enavarro51 SX and SXdg should be red, their matrix representation is not diagonal. I updated the list above 👍

@galeinston galeinston changed the title Tweak colors [WIP] Tweak colors Sep 19, 2020
@enavarro51
Copy link
Contributor

Classical: Dark blue
I, X, CX, CCX, C3X, C4X, any MCX, DCX, Swap, CSwap, and controlled versions of these

@Cryoris Some (hopefully) last questions on colors.

  • c3x and c4x are no longer active. You can't enter them as circuit.c3x and if you create a 3-control x, it returns mcx as the name.
  • if you enter circuit.mcx it returns mcx_gray as the name. Should we add this to the color list?
  • DCX, for example, can have an unlimited number of controls, so c6dcx could be a gate name. What we're trying to do here is a simple mapping of gate names to colors, which is what we have always done. Is it ok if we stop the number of controls for Dark Blue at some reasonable number like 2 (for other than X). So cdcx and ccdcx would be DarkBlue and any higher # of controls would be DarkRed? Similar for Swap?

@galeinston
Copy link
Contributor Author

@Cryoris I also have a question regarding the R-gate (as shown in the picture). I can't find it neither in Circuit Library document nor in IQX page, can you clarify if this gate has been deprecated/deleted?

image

@1ucian0 1ucian0 marked this pull request as draft September 22, 2020 20:21
@enavarro51
Copy link
Contributor

enavarro51 commented Sep 23, 2020

We've made the following changes in this PR.

  • First, all changes should be 100% backward compatible. We've only added functionality, not taken any away.
  • There are now 3 color styles available - default, iqx, and bw. They can be set in one of 2 ways.
    • In the [Default] section of the settings.conf file in the ~/.qiskit dir, add circuit_mpl_style = '<style_name>'
    • In the draw() command add param style={'name': '<style_name>'}
  • There needed to be some special mpl drawer code added to handle controlled classical gates, so iqx will not be as generic as other possible styles. The problem was with gate names like c5dcx, c6dcx, and onward.
  • The dispcol dict in the style dict now sets colors as (gate_color, text_color). This allows the use of light and dark gate colors with black or white text. Actually someone could configure the style dict to do any combo, such as dark blue text on a yellow gate. The previous dispcol format of gate_color only still works, and will in that case use the 'gatetextcolor' for text.
  • The style dict docs in quantumcircuit.py and circuit_visualization.py have been updated with the current dispcol, with explanation changes as well.
  • A binder test was added for the iqx colors.
  • A release note will be done once any replies to this comment have been addressed.

Phase 2 for the future,

  • This PR creates 3 separate style classes in qcstyle.py. We envision that in future, this can be reduced to 1 default class and the additional styles would be read in from json files. I don't believe users should be allowed to modify these 3 styles directly, but they can copy these and create new styles of their own. Which brings up where to put things in the future.
    • I believe that default, iqx, and bw json style files should be part of qiskit, perhaps in the visualization directory.
    • User json files should go in a fixed place, perhaps in a styles dir under ~/.qiskit.
  • The code for reading in the json files already exists, though there is only minimal error checking currently. Deciding on the locations for the files is the main issue.
  • We might want to look at any possible special cases, like the controlled classical gates in iqx and see if any of these can be addressed in a generic way. Right now all a user can do with color, is change the colors of the specific gate names in dispcol.

@1ucian0 1ucian0 marked this pull request as ready for review September 24, 2020 21:01
@1ucian0 1ucian0 changed the title [WIP] Tweak colors Tweak colors Sep 24, 2020
@Qiskit Qiskit deleted a comment from enavarro51 Sep 28, 2020
Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tweak colors to match new IQX colors

6 participants